Skip to content

Conversation

cgwalters
Copy link
Collaborator

No description provided.

@bootc-bot bootc-bot bot requested a review from jeckersb October 2, 2025 18:09
@github-actions github-actions bot added area/install Issues related to `bootc install` area/documentation Updates to the documentation labels Oct 2, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant amount of work related to composefs, including switching the fs-verity hashing algorithm from SHA256 to SHA512, adding a new workflow for building 'sealed' images with Unified Kernel Images (UKIs), and making the composefs backend a default feature. The changes are mostly consistent and well-structured, with good use of type aliases to improve code clarity.

However, I've found a few issues that need attention:

  • There's some dead and potentially buggy code in the new xtask for building sealed images.
  • A new Dockerfile contains a leftover command that should be removed.
  • The documentation for newly added CLI options and subcommands is incomplete.

Please see my detailed comments for suggestions on how to address these points.

Comment on lines +117 to +131
**--composefs-native**



**--insecure**



Default: false

**--bootloader**=*BOOTLOADER*



Default: grub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation for the new options --composefs-native, --insecure, and --bootloader is incomplete. Please add descriptions for these options to explain their purpose and usage.

| **bootc usr-overlay** | Add a transient writable overlayfs on `/usr` |
| **bootc install** | Install the running container to a target |
| **bootc container** | Operations which can be executed as part of a container build |
| **bootc composefs-finalize-staged** | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for the new subcommand bootc composefs-finalize-staged is missing. Please add a description to explain what this command does.

@cgwalters
Copy link
Collaborator Author

Split prep work for this to #1665

@cgwalters cgwalters force-pushed the composefs-by-default branch from 837dc44 to cccc2f8 Compare October 6, 2025 14:10
@cgwalters
Copy link
Collaborator Author

@jeckersb I ended up reworking this one to use the bootc-in-container for the sealing side because it would be too annoying in our CI to have it be on the host (which is ubuntu here).

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 6, 2025

I'm guessing this is something to do with me running just build-sealed inside of my toolbox, but I'm getting...

$ podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=target/test-secureboot/db.key --secret=id=cert,src=target/test-secureboot/db.crt -f Dockerfile.cfsuki .
Error: failed to parse query parameter 'secrets': "[\"id=key,src=podman-build-secret3148315696\",\"id=cert,src=podman-build-secret3059655711\"]": rename /var/tmp/libpod_builder3803266185/build/podman-build-secret3148315696 /var/tmp/libpod_builder3803266185/podman-build-secret3148315696: no such file or directory
error: command exited with non-zero code `podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=target/test-secureboot/db.key --secret=id=cert,src=target/test-secureboot/db.crt -f Dockerfile.cfsuki .`: 125
error: Recipe `build-sealed` failed on line 18 with exit code 1

@cgwalters
Copy link
Collaborator Author

Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for podman in the toolbox look like?

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for podman in the toolbox look like?

Nope still doesn't work with absolute paths

⬢ [jeckersb@toolbx bootc]$ ls -l /var/home/jeckersb/git/bootc/target/test-secureboot/db.{crt,key}
-rw-r--r--. 1 jeckersb jeckersb 1854 Oct  6 15:54 /var/home/jeckersb/git/bootc/target/test-secureboot/db.crt
-rw-------. 1 jeckersb jeckersb 3272 Oct  6 15:54 /var/home/jeckersb/git/bootc/target/test-secureboot/db.key
⬢ [jeckersb@toolbx bootc]$ podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=/var/home/jeckersb/git/bootc/target/test-secureboot/db.key --secret=id=cert,src=/var/home/jeckersb/git/bootc/target/test-secureboot/db.crt -f Dockerfile.cfsuki .
Error: failed to parse query parameter 'secrets': "[\"id=key,src=podman-build-secret2171653322\",\"id=cert,src=podman-build-secret2161445125\"]": rename /var/tmp/libpod_builder2211788443/build/podman-build-secret2171653322 /var/tmp/libpod_builder2211788443/podman-build-secret2171653322: no such file or directory

I don't have any wrapper for podman in my toolbox, but I conditionally set CONTAINER_CONNECTION to go over the podman socket. From .bashrc:

if [ -n "${TOOLBOX_PATH}" ]
then
    CONTAINER_CONNECTION=host
fi

and then...

$ podman system connection list
Name        URI                                       Identity    Default     ReadWrite
host        unix:///run/user/1000/podman/podman.sock              true        true
$ systemctl --user status podman.socket
● podman.socket - Podman API Socket
     Loaded: loaded (/usr/lib/systemd/user/podman.socket; enabled; preset: disabled)
     Active: active (listening) since Mon 2025-10-06 21:50:56 EDT; 13min ago
 Invocation: 0257bbab67374412a75a2c0b32b77018
   Triggers: ● podman.service
       Docs: man:podman-system-service(1)
     Listen: /run/user/1000/podman/podman.sock (Stream)
     CGroup: /user.slice/user-1000.slice/[email protected]/app.slice/podman.socket

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Given the eyeball test the changes look good to me. From CI it looks like it needs a cargo fmt and an update to test image pull check since now we're pulling the image which it expects to be absent?

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly.

So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds.

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly.

So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds.

Ahhhhhh it's this - containers/podman#25314

I'll put in a separate PR to add that silly workaround to our .dockerignore

@cgwalters cgwalters force-pushed the composefs-by-default branch 5 times, most recently from 3dbc6dd to ee05675 Compare October 8, 2025 17:42
@cgwalters
Copy link
Collaborator Author

cgwalters commented Oct 8, 2025

OK it took me a bit too long to figure out the reason that we were getting cargo: command not found is that the default GHA Ubuntu runners seem to install Rust via rustup in the default runner user, so it isn't available as root.

This will get naturally fixed when we move to using bcvk here.

However...for now I moved the outer portion of the sealing back to shell script - we've already moved some of the inner portion into the Rust code.

@cgwalters cgwalters force-pushed the composefs-by-default branch from ee05675 to a58d0a3 Compare October 8, 2025 18:22
@cgwalters
Copy link
Collaborator Author

OK though, unfortunately right now tmt's virtual provisioner doesn't support uefi. That looks pretty easy to fix, but I'm a bit tempted to try out having bcvk libvirt replace the testcloud stuff.

@cgwalters cgwalters enabled auto-merge (rebase) October 8, 2025 19:04
@cgwalters cgwalters requested a review from jeckersb October 8, 2025 19:04
@cgwalters cgwalters disabled auto-merge October 8, 2025 19:38
@cgwalters cgwalters force-pushed the composefs-by-default branch 4 times, most recently from bb56008 to b70bee9 Compare October 9, 2025 13:36
@cgwalters cgwalters force-pushed the composefs-by-default branch 4 times, most recently from fda247e to 10b2832 Compare October 9, 2025 22:00
@jeckersb
Copy link
Collaborator

OK though, unfortunately right now tmt's virtual provisioner doesn't support uefi. That looks pretty easy to fix, but I'm a bit tempted to try out having bcvk libvirt replace the testcloud stuff.

Is that the primary thing blocking this at this point?

Signed-off-by: Colin Walters <[email protected]>
This ensures we're SHA-512 across the board.

Signed-off-by: Colin Walters <[email protected]>
- Use bash strict mode more consistently
- Drop the error redirections which can mask problems as
  recommended by AI

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters force-pushed the composefs-by-default branch 2 times, most recently from f500118 to 39fffbd Compare October 16, 2025 13:32
@cgwalters
Copy link
Collaborator Author

Is that the primary thing blocking this at this point?

Yeah I think so, filed teemtee/tmt#4203

That said, we can work around this by using bcvk to provision a system external to tmt. That's not hard, but the downside is that it's logic that'd need to be replicated into anything else that wants to use tmt.

@cgwalters cgwalters force-pushed the composefs-by-default branch from 39fffbd to 1c4b1d9 Compare October 16, 2025 13:47
@jeckersb
Copy link
Collaborator

This looks good testing it out on my end, looks like CI is failing because bootupd is missing on the ostree case. I think maybe the jobs are stepping on each other trying to both use the localhost/bootc tag? Maybe just needs the Justfile updated to use localhost/bootc-sealed in the sealed case...

@cgwalters cgwalters force-pushed the composefs-by-default branch from 1c4b1d9 to c5c0137 Compare October 16, 2025 14:37
@cgwalters cgwalters enabled auto-merge (rebase) October 16, 2025 14:47
@cgwalters
Copy link
Collaborator Author

This looks good testing it out on my end, looks like CI is failing because bootupd is missing on the ostree case.

Yeah I'd messed up the bootupd detection, fixed now

Copy link
Collaborator

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍, assuming the in-flight jobs pass just needs trivial validation cleanup

- Change the install logic to detect UKIs and automatically
  enable composefs
- Change the install logic to detect absence of bootupd
  and default to installing systemd-boot
- Move sealing bits to the toplevel
- Add Justfile entrypoints
- Add basic end-to-end CI coverage (install + run) using
  our integration tests
- Change lints to ignore `/boot/EFI`

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters force-pushed the composefs-by-default branch from c5c0137 to 77685f0 Compare October 16, 2025 15:20
@cgwalters
Copy link
Collaborator Author

Holy 🐮 so I removed the background & on the rm -rf in the setup as it felt obscure to me but...the "bootc ubuntu setup" in some of these jobs is now over 1h30m! In other runs it's just 7-9m.

Of course, us having to clean up tons of garbage from the stock runners is ridiculous and hopefully GH will implement #1669

But while let's not kill CI for this one, I'm going to try to go back to optimizing the provisioner...

@cgwalters
Copy link
Collaborator Author

OK no, it's just 2/4 jobs here that had some kind of pathological slowness going on, but we don't have enough timing information to have a good idea of what specifically that was. Working on a followup commit.

@cgwalters cgwalters merged commit f4c678e into bootc-dev:main Oct 16, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation Updates to the documentation area/install Issues related to `bootc install`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants